feat(mcp): comma-split note_types/entity_types/categories in search_notes#962
Conversation
…otes (#930) Add `parse_str_list` to utils.py — like `parse_tags` but without stripping '#' — and wire it as the BeforeValidator for note_types, entity_types, and categories in search_notes. This makes passing "note,task" or '["note","task"]' work correctly instead of being wrapped as a single literal value by coerce_list. coerce_list is left unchanged; canvas and other callers that depend on its wrap-single-string behaviour are unaffected. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Drew Cain <groksrc@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 736aa840aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # parse_str_list, not coerce_list: "note,task" must split into ["note", "task"] | ||
| # consistent with how tags are handled (#910/#930). coerce_list wraps the whole | ||
| # comma string as the single literal type ["note,task"], which matches nothing. | ||
| BeforeValidator(parse_str_list), |
There was a problem hiding this comment.
This validator only runs when FastMCP/Pydantic validates a tool call, but the CLI path in src/basic_memory/cli/commands/tool.py invokes mcp_search(...) directly with repeatable options. In that path bm tool search-notes --type note,task reaches the function as note_types=["note,task"], skips this BeforeValidator, and line 904 still forwards the literal type "note,task", which matches nothing. Tags already compensate with an in-body parse_tags(tags) for the same direct-call reason; these new comma-split filters need equivalent normalization in the function body.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified and fixed in commit 0f1d728.
Claim confirmed: BeforeValidator(parse_str_list) only fires through the MCP/Pydantic path. The CLI path (cli/commands/tool.py) calls search_notes() directly, bypassing it. bm tool search-notes --type note,task arrived as note_types=["note,task"] and matched nothing, while tags already had in-body parse_tags() normalization for the same reason.
Fix: Added in-body parse_str_list() calls for note_types, entity_types, and categories in the search_notes function body, mirroring the parse_tags(tags) pattern. The validators remain for the MCP path; the body normalization covers direct callers. parse_str_list is idempotent so already-split lists pass through unchanged.
Tests added: annotation-level comma-split tests for all three params, and an async direct-call regression test (test_search_notes_direct_call_splits_comma_note_types) that calls search_notes() directly with note_types=["note,task"] and confirms it matches a real note.
In reply to: #962 (comment)
| for raw in v | ||
| if raw is not None | ||
| for item in str(raw).split(",") |
There was a problem hiding this comment.
When a client sends a JSON array string or list containing non-string values, e.g. [42] or ["entity", 42], this branch stringifies each element before Pydantic sees it, so the call is accepted as filters like ["42"] instead of being rejected as invalid input. That regresses the previous coerce_list behavior and the stricter strict_search_tags boundary, causing bad caller data to become silent no-result searches; return the original value when any list element is not a string before doing comma splitting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified and fixed in commit 0f1d728.
Claim confirmed: The list branch in parse_str_list used str(raw).split(",") which silently coerced [42] to ["42"] before Pydantic saw it, accepting invalid input as a no-result search. This regressed the strict boundary established by strict_search_tags for tags.
Fix: Added a non-string element guard at the top of the list branch in parse_str_list (utils.py). If any list element is not a string (ignoring None), the original value is returned unchanged so Pydantic rejects it with a clear validation error. Also changed str(raw).split(",") to raw.split(",") in the all-string fast path so the now-redundant str() cast is removed. All-string lists and comma-string-in-list inputs continue to work correctly.
Tests added: Three new test methods in TestParseStrList (test_coerce.py): test_int_list_passthrough_for_pydantic_rejection, test_mixed_list_passthrough_for_pydantic_rejection, test_dict_list_passthrough_for_pydantic_rejection. Three new annotation-level rejection tests in test_tool_search.py for note_types, entity_types, and categories, each confirming [42] raises ValidationError.
In reply to: #962 (comment)
… path and reject non-string list elements Codex review of PR #962 identified two real issues: 1. CLI bypass: the BeforeValidator(parse_str_list) on note_types, entity_types, and categories only fires through MCP/Pydantic validation. The CLI path in cli/commands/tool.py calls search_notes() directly, so `bm tool search-notes --type note,task` arrived as note_types=["note,task"] and matched nothing. Fix: add in-body parse_str_list() normalization for all three params (mirroring the existing parse_tags() call for tags on the same code path). 2. Silent stringify: parse_str_list used str(raw) in the list branch, so [42] became ["42"] before Pydantic saw it, accepting invalid input as a no-result search instead of rejecting it. Fix: guard against non-string list elements and return the original value unchanged so Pydantic rejects it with a clear error. Tests added: annotation-level split tests for note_types/entity_types/categories, non-string-element rejection tests, async direct-call regression for note_types, and unit-level parse_str_list non-string list tests in test_coerce.py. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Drew Cain <groksrc@gmail.com>
1 participant
Closes #930
Summary
Implements Option A (comma-split) for the
note_types,entity_types, andcategoriesparameters insearch_notes, allowing callers to pass either a plain string ("note,canvas"), a JSON-array string ('["note","canvas"]'), or a proper list — all normalised tolist[str]before validation.Changes
src/basic_memory/utils.py— addedparse_str_list(v): handlesNone → [], comma-split strings, JSON-array strings, and lists-with-comma-elements, without stripping#prefixes (unlikeparse_tags). Non-str/list/None values pass through unchanged so Pydantic can still reject them.src/basic_memory/mcp/tools/search.py— replaced thecoerce_listBeforeValidatoronnote_types,entity_types, andcategorieswithparse_str_list. Removed the now-unusedcoerce_listimport.coerce_listitself is unchanged, so canvas and other callers are unaffected.tests/test_coerce.py— addedTestParseStrListcovering:None, single string, comma string, spaces+comma string, JSON-array string (single/multi/comma-element), plain list, list-with-comma-element, multi-comma-element list,#-prefix preservation, and passthrough forint/dict.Testing
All 41 tests pass;
rufflint clean.Review notes
parse_str_list([['a','b'],'c'])produces messy stringified output["['a'", "'b']", 'c']because each element isstr()'d before comma-splitting. This is an unrealistic input not in scope and consistent withparse_tags-style handling, so not blocking — but a future hardening could flatten nested lists.TestParseStrListcases (the earlier GH comment correctly said 17). Cosmetic discrepancy only; all tests pass.🤖 Generated with Claude Code